Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "Fix co-assignment for binary operations" #6985

Merged
merged 1 commit into from
Sep 2, 2022
Merged

Revert "Fix co-assignment for binary operations" #6985

merged 1 commit into from
Sep 2, 2022

Conversation

gjoseph92
Copy link
Collaborator

This reverts commit 7ebd1d9.

Re-opens #6597. See coiled/benchmarks#295 for motivation.

  • Tests added / passed
  • Passes pre-commit run --all-files

@gjoseph92 gjoseph92 self-assigned this Sep 1, 2022
@gjoseph92 gjoseph92 mentioned this pull request Sep 1, 2022
Closed
5 tasks
@gjoseph92 gjoseph92 requested a review from fjetter September 1, 2022 16:51
gjoseph92
gjoseph92 commented Sep 1, 2022
View reviewed changes
distributed/scheduler.py
tg.last_worker = (
ws if tg.states["released"] + tg.states["waiting"] > 1 else None
)
tg.last_worker_tasks_left -= 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, what it looked like before #6985:

distributed/distributed/scheduler.py

Lines 1850 to 1868 in f07f384

ws = tg.last_worker
if not (ws and tg.last_worker_tasks_left and ws.address in self.workers):
# Last-used worker is full or unknown; pick a new worker for the next few tasks
ws = min(
(self.idle or self.workers).values(),
key=partial(self.worker_objective, ts),
)
assert ws
tg.last_worker_tasks_left = math.floor(
(len(tg) / self.total_nthreads) * ws.nthreads
)
# Record `last_worker`, or clear it on the final task
tg.last_worker = (
ws if tg.states["released"] + tg.states["waiting"] > 1 else None
)
tg.last_worker_tasks_left -= 1
return ws

Note there's still a slight change here—previously we'd schedule onto any worker if none were idle, now we only schedule onto running workers.

jrbourbeau
jrbourbeau reviewed Sep 1, 2022
View reviewed changes
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gjoseph92! Have we had a chance to confirm this fixes the observed regression in coiled/benchmarks#295 as expected? Also cc @ian-r-rose in case he's looked into it

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2022

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       14 files   -        1         14 suites   - 1   6h 9m 1s ⏱️ - 23m 51s
  3 082 tests  -        4    2 997 ✔️  -        5    84 💤 ±  0  1 ❌ +1 
21 569 runs   - 1 265  20 572 ✔️  - 1 277  996 💤 +11  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 3f215fd. ± Comparison against base commit bfc5cfe.

@gjoseph92
Copy link
Collaborator Author

Have we had a chance to confirm this fixes the observed regression

I have not, but will try now.

@gjoseph92
Copy link
Collaborator Author

I've run it manually and confirmed it fixes the regression. I can't figure out how to get it to render with the dashboard script, but here are memory profiles. Notice the difference in peak values:

Before this PR:
after-steal

After this PR:
a5813cfa

fjetter
fjetter approved these changes Sep 2, 2022
View reviewed changes
Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gjoseph92

@fjetter fjetter merged commit acf6078 into dask:main Sep 2, 2022
@jrbourbeau
Copy link
Member

Yeah, thanks for handling this @gjoseph92

@fjetter fjetter mentioned this pull request Sep 2, 2022
Open
@gjoseph92 gjoseph92 deleted the revert-coassignment-fix branch September 6, 2022 18:06
@gjoseph92 gjoseph92 mentioned this pull request Sep 6, 2022
Closed
gjoseph92 added a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022
@gjoseph92
Revert "Fix co-assignment for binary operations" (dask#6985)
75e7378 
This reverts commit 7ebd1d9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrbourbeau jrbourbeau jrbourbeau left review comments

@fjetter fjetter fjetter approved these changes

Assignees

@gjoseph92 gjoseph92

Labels
memory performance regression
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gjoseph92 @jrbourbeau @fjetter